BUGFIX: Flush node content caches without workspace name #5122
Conversation
| return self::forNodeAggregate( | ||
| $node->contentRepositoryId, | ||
| null, | ||
| $node->aggregateId | ||
| ); |
There was a problem hiding this comment.
...change this to sth like
| return self::forNodeAggregate( | |
| $node->contentRepositoryId, | |
| null, | |
| $node->aggregateId | |
| ); | |
| return new self( | |
| 'Node_' | |
| . self::getHashForContentRepositoryId($contentRepositoryId) | |
| . '_' . $nodeAggregateId->value | |
| ); |
and we could reduce the amount of repetition, by introducing some private fromParts constructor, e.g.:
return self::fromParts(self::NODE_PREFIX, self::hashed($contentRepositoryId), $nodeAggregateId->value);that would also make it easier to change our internal pattern because I think:
<prefix>_sha1(<workspace>@<cr>)_<aggregateId> is not safe (because the NodeAggregateId could contain underscores for example – not currently, but it is being requested)
<prefix>|sha1(<workspace>)|sha1(<cr>)|<aggregateId> would make more sense to me – even though: I don't understand why we randomly(?) use sha1 for 2 of those?
There was a problem hiding this comment.
The sha1 is to reduce/fix the length of the key.
is not safe (because the NodeAggregateId could contain underscores for example
There is also no need to change this, as this is just a visual delimiter, without any meaning.
There was a problem hiding this comment.
The sha1 is to reduce/fix the length of the key
It was used to avoid special characters to render the tags inconsistent – but this should no longer be an issue with our restrictive ValueObjects.
Re length:
The max length of a cache tag is 250 characters.
SHA1 hashes are 40 characters long.
workspaceName can be up to 200 characters
cr id only 16 (i.e. hashing the cr ID increases its length)
We could just hash the complete tag, but that would obviously make it impossible to read so I would not suggest to do so.
Instead I'd say (sorry, this is not related to your change, but since we talk about it):
- Reduce the length of
WorkspaceNamedramatically – 200 characters is crazy - use a delimiter that can' be part of aggregate id, workspace name or cr id
- don't hash the parts at all
There is also no need to change this, as this is just a visual delimiter, without any meaning
Well, it's not just visual, imagine a node aggregate id of foo_notAWorkspace :)
There was a problem hiding this comment.
Sure, but can we move this into a dedicated issue?
There was a problem hiding this comment.
created an issue because the sha1 bugged me while debugging: #5164
There was a problem hiding this comment.
@dlubitz two more little issues – I'll push a PR-PR with some suggestions
|
I still like bastis comment about the bool/ second WorkspaceName but otherwise wonderful thanks for sifting through this! |
|
For some reason I seem to have ended up on an older commit 🙈 Great |
As we don't want to iterate over all workspaces on flushing content caches, caused by asset changes, we add addionally node cache tags without a workspace name. So we can flush all node aggregates in all workspaces with just the node aggregate id.